fix: record recovered local address by kyessenov · Pull Request #13581 · envoyproxy/envoy

kyessenov · 2020-10-14T22:31:32Z

Signed-off-by: Kuat Yessenov kuat@google.com

Commit Message: fixes #13554
Risk Level: low (bug fix)
Testing: unit
Docs Changes: none
Release Notes:
Platform Specific Features:

Signed-off-by: Kuat Yessenov <kuat@google.com>

lambdai · 2020-10-14T22:38:17Z

source/server/connection_handler_impl.cc


 void ConnectionHandlerImpl::ActiveTcpListener::newConnection(
    Network::ConnectionSocketPtr&& socket, std::unique_ptr<StreamInfo::StreamInfo> stream_info) {
+  // Update local address in case it is restored by a listener filter


Can you add the TODO? This is good since we known original_dst could change setDownstreamLocalAddress but we don't know if other crazy listener filter could do (proxy protocol?)

Without exposing stream info to listener filters, we just have to fix this case by case. Is there something else that is changed by listener filters?

TransportProtocol applicationProtocol. It depends on the overlap of stream info and socket info.
Also newConnection() is not the only termination of the socket. For a closed socket the small discrepancy is not ideal but should be fine for waiting for the further solution.

I'm having some trouble understanding the issue being fixed in this PR. Should original_dst be calling setDownstreamLocalAddress? I'm guessing that the answer is no.

I'm concerned that this change breaks other intended behavior, but I can see the counter argument involving no existing tests breaking. That said, a lot of functionality is covered by small tests, which wouldn't detect breakages due to changes like this one which involve interaction between multiple extensions.

The history is that this is a regression. Prior to #13554, stream info was instantiated after listener filters, so the value of downstream local address is the modified value. After that PR, stream info is instantiated before listener filters, so the value has changed. This is a fix to restore the original behavior.

The real issue is lack of any integration testing for original_dst. I think we would have caught the regression sooner if we had any test that exercised original_dst and logging. I don't know why it's like that, maybe it was difficult to set up iptables in a test.

Is socket->localAdress() the actual address coming from the socket or a modified value set by orig dst filter?

Basically, I'm having trouble understanding new behavior based on the comment added here.

It's the modified value AFAICT.

Can you update the comment to something like:

// Refresh local address in case it was restored by a listener filter like the original dst filter.

lambdai

Kill the known bug with fire!

antoniovicente · 2020-10-16T14:43:27Z

source/server/connection_handler_impl.cc


 void ConnectionHandlerImpl::ActiveTcpListener::newConnection(
    Network::ConnectionSocketPtr&& socket, std::unique_ptr<StreamInfo::StreamInfo> stream_info) {
+  // Update local address in case it is restored by a listener filter


I'm having some trouble understanding the issue being fixed in this PR. Should original_dst be calling setDownstreamLocalAddress? I'm guessing that the answer is no.

I'm concerned that this change breaks other intended behavior, but I can see the counter argument involving no existing tests breaking. That said, a lot of functionality is covered by small tests, which wouldn't detect breakages due to changes like this one which involve interaction between multiple extensions.

antoniovicente · 2020-10-16T14:59:43Z

test/server/connection_handler_test.cc

+          Invoke([&](const Http::RequestHeaderMap*, const Http::ResponseHeaderMap*,
+                     const Http::ResponseTrailerMap*, const StreamInfo::StreamInfo& stream_info) {
+            EXPECT_EQ(alt_address, stream_info.downstreamLocalAddress());
+          }));


It would be good to exercise this fix in an integration test to fully capture the full implications.

Agree, but there is no integration test for original_dst, unfortunately.

Could you work with the owners of original_dst to improve test coverage?

Who's the owner of original_dst? It's not in CODEOWNERS.

I don't know who originally wrote the code (someone in Istio I'm sure). The problem is I'm not sure we can have an integration test that is sandboxed. It requires iptables. If this is possible we should do it.

Signed-off-by: Kuat Yessenov <kuat@google.com>

…ress

antoniovicente · 2020-10-19T17:37:25Z

@mattklein123 Want to take a look or should I merge once CI is green?

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2020-10-19T19:44:59Z

/retest

repokitteh-read-only · 2020-10-19T19:45:03Z

Retrying Azure Pipelines.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13581 (comment) was created by @kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

* master: (22 commits) ci: various improvements (envoyproxy#13660) dns: fix defunct fd bug in apple resolver (envoyproxy#13641) build: support ppc64le with wasm (envoyproxy#13657) [fuzz] Added random load balancer fuzz (envoyproxy#13400) dependencies: compute and check release dates via GitHub API. (envoyproxy#13582) mac ci: try ignoring update failure (envoyproxy#13658) watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. (envoyproxy#13103) typos: fix a couple 'enovy' mispellings (envoyproxy#13645) lua: Expose stream info downstreamLocalAddress and downstreamDirectRemoteAddress for Lua filter (envoyproxy#13536) tap: fix upstream streamed transport socket taps (envoyproxy#13638) Revert "delay health checks until transport socket secrets are ready. (envoyproxy#13516)" (envoyproxy#13639) Watchdog: use abort action as a default if killing is enabled. (envoyproxy#13523) [fuzz] Fixed divide by zero bug (envoyproxy#13545) wasm: flip the meaning of the "repository" in envoy_wasm_cc_binary(). (envoyproxy#13621) fix: record recovered local address (envoyproxy#13581) docs: fix incorrect compressor filter doc (envoyproxy#13611) docs: clean up docs for azp migration (envoyproxy#13558) wasm: fix building Wasm example. (envoyproxy#13619) test: Refactor flood tests into a separate test file (envoyproxy#13556) wasm: re-enable tests with precompiled modules. (envoyproxy#13583) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

kyessenov added 2 commits October 14, 2020 15:28

fix: record recovered local address

7bad421

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix test

9a3fe80

Signed-off-by: Kuat Yessenov <kuat@google.com>

lambdai reviewed Oct 14, 2020

View reviewed changes

lambdai previously approved these changes Oct 14, 2020

View reviewed changes

htuch assigned antoniovicente Oct 15, 2020

yangminzhu mentioned this pull request Oct 16, 2020

update proxy sha istio/istio#27917

Closed

antoniovicente reviewed Oct 16, 2020

View reviewed changes

antoniovicente added the waiting:any label Oct 16, 2020

repokitteh-read-only bot removed the waiting:any label Oct 16, 2020

kyessenov added 2 commits October 19, 2020 10:35

review

7fe3fcf

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into update_socket_add…

f368f32

…ress

kyessenov dismissed lambdai’s stale review via f368f32 October 19, 2020 17:35

antoniovicente previously approved these changes Oct 19, 2020

View reviewed changes

fix format

ca9e25a

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed antoniovicente’s stale review via ca9e25a October 19, 2020 18:05

mattklein123 approved these changes Oct 19, 2020

View reviewed changes

antoniovicente approved these changes Oct 19, 2020

View reviewed changes

antoniovicente merged commit 7915659 into envoyproxy:master Oct 19, 2020

kyessenov added a commit to istio/envoy that referenced this pull request Oct 19, 2020

fix: record recovered local address (envoyproxy#13581)

8a5dca3

Signed-off-by: Kuat Yessenov <kuat@google.com>

bianpengyuan pushed a commit to istio/envoy that referenced this pull request Oct 19, 2020

fix: record recovered local address (envoyproxy#13581) (#277)

4760a0f

Signed-off-by: Kuat Yessenov <kuat@google.com>

Conversation

kyessenov commented Oct 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovicente commented Oct 19, 2020

Uh oh!

kyessenov commented Oct 19, 2020

Uh oh!

repokitteh-read-only bot commented Oct 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants